Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(install): Separate namespaced and descoped rbacs #4914

Merged
merged 4 commits into from
Dec 13, 2023
Merged

feat(install): Separate namespaced and descoped rbacs #4914

merged 4 commits into from
Dec 13, 2023
+1,118 −121

Conversation

gansheer
Copy link
Contributor

@gansheer gansheer commented Nov 10, 2023 •
edited
Loading

Ref #3165

  • Update config rbacs files to have separate full namespaced and global (descoped) version.
  • Adapt the CLI and kustomize installation

Release Note

feat(install): Separate namespaced and descoped rbacs

@gansheer gansheer force-pushed the feat/3165_namespaced_descoped_rbacs branch 2 times, most recently from 5a3717b to f4eb25d Compare November 13, 2023 09:30
@gansheer gansheer force-pushed the feat/3165_namespaced_descoped_rbacs branch 4 times, most recently from 8cb24ae to 405f61a Compare November 13, 2023 16:15
@gansheer gansheer force-pushed the feat/3165_namespaced_descoped_rbacs branch 3 times, most recently from 582cd20 to b3be16f Compare November 16, 2023 07:59
@gansheer
Copy link
Contributor Author

@oscerd @christophd @squakez Could you trigger the e2e tests please 🙏

@gansheer gansheer force-pushed the feat/3165_namespaced_descoped_rbacs branch from b3be16f to c89bcaf Compare November 17, 2023 13:12
@github-actions GitHub Actions
Copy link
Contributor

🐫 Thank you for contributing!

Code Coverage Report ⚠️ - Coverage changed: 33.6% --> 33.5% (Coverage difference: -.1%)

@gansheer gansheer force-pushed the feat/3165_namespaced_descoped_rbacs branch from c89bcaf to e55f456 Compare November 20, 2023 09:57
@github-actions GitHub Actions
Copy link
Contributor

🐫 Thank you for contributing!

Code Coverage Report ⚠️ - Coverage changed: 33.6% --> 33.5% (Coverage difference: -.1%)

@gansheer gansheer force-pushed the feat/3165_namespaced_descoped_rbacs branch from e55f456 to 084d6b4 Compare November 20, 2023 13:29
@github-actions GitHub Actions
Copy link
Contributor

🐫 Thank you for contributing!

Code Coverage Report ⚠️ - Coverage changed: 33.6% --> 33.5% (Coverage difference: -.1%)

@gansheer gansheer force-pushed the feat/3165_namespaced_descoped_rbacs branch from 084d6b4 to 885d6d6 Compare December 5, 2023 08:45
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 5, 2023

🐫 Thank you for contributing!

Code Coverage Report ⚠️ - Coverage changed: 33.6% --> 33.5% (Coverage difference: -.1%)

@gansheer gansheer force-pushed the feat/3165_namespaced_descoped_rbacs branch 3 times, most recently from feeb941 to 4e1ec52 Compare December 11, 2023 17:07
@gansheer gansheer marked this pull request as ready for review December 11, 2023 17:10
@gansheer
Copy link
Contributor Author

@squakez can you launch the tests again please 🙏 ?

@squakez
Copy link
Contributor

squakez commented Dec 12, 2023

@gansheer is this good to merge? how is this affecting the installation procedures (if it does affect in any manner)?

@gansheer
Copy link
Contributor Author

It is ready for merge. From my tests, on vanilla kubernetes and Openshift, it is transparent on existing installation methods CLI and Kustomize (I didn't test helm as helm uses its own files).

squakez
squakez reviewed Dec 12, 2023
View reviewed changes
config/rbac/kustomization.yaml
- operator-role-binding-local-registry.yaml


transformers:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this one something which would require user's attention by any chance? I mean, does it deserve some kind of documentation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually this is a fix. This RoleBinding should not have been transformed in a ClusterRoleBinding in the first place as it is supposed to only give access to a configmap in the "kube-public" namespace.

The current documentation only reference the "high-level" kustomization files (or the install script https://camel.apache.org/camel-k/2.1.x/installation/advanced/kustomize.html), but you are right I will update this part to add some explanation.

In any case a lot of documentation for specific cases are lacking in favor of the script usage.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@squakez I added the documentation 📖

@gansheer gansheer force-pushed the feat/3165_namespaced_descoped_rbacs branch from ed5fb7e to 17ffb5b Compare December 12, 2023 21:13
squakez
squakez approved these changes Dec 13, 2023
View reviewed changes
Copy link
Contributor

@squakez squakez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good for now, but we need to provide some simplification in the future.

@squakez squakez merged commit e6fcd0f into apache:main Dec 13, 2023
15 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@claudio4j claudio4j claudio4j left review comments

@squakez squakez squakez approved these changes

@oscerd oscerd oscerd approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gansheer @squakez @claudio4j @oscerd